Remove 0-size and variable size array members #13945

yuyichao · 2015-11-11T01:35:55Z

For some reason, the llvm-config from LLVM 3.7 adds -pedantic to cflags and cxxflags. This causes a huge amount of compiler warnings (mainly because of JL_DATA_TYPE). I couldn't find out why did LLVM starts doing this or how to workaround it in general so I decided to fix most of the warnings....

This PR doesn't completely fix all the -Wpedantic warnings (the ones that are not fixed is anonymous structs which we use for bitsfields in a union. This turns out to be valid C11 but not valid C++ yet...).

This could break embedding. In particular,

jl_value_t is only a declaration now. Direct use of jl_value_t instead of jl_value_t* shouldn't be necessary.
Types with variable array members do no have such member anymore. The address of the variable length array is calculated directly with pointer arithmetics instead. Notes about how each of them should be updated,
1. jl_value_t::fieldptr
  
  We already have jl_data_ptr and this macro should be used instead.
2. jl_sym_t::data
  
  Use jl_symbol_name, which is also the name of the exported function.
3. jl_taggedvalue_t::value
  
  (not actually a flexible array member but was serving a similar purpose). I doubt any embedding code manipulate jl_taggedvalue_t directly but jl_valueof is provided as a convenient way to access the value pointer.
4. jl_svec_t::data
  
  We already have jl_svec_data and this macro should be used instead.
5. jl_datatype_t::fields
  
  This shouldn't be accessed directly since Variable size field descriptors #13147. jl_datatype_fields macro is also added to get the address of the fields in the rare case it is needed.
If any code was relying on JL_DATA_TYPE in a non-gc allocated struct to provide necessary alignment, it needs to find another way to do it. (define its own zero length array member, use the alignment attributes etc.)
The presence of jl_valueof macro and function could be used as a compile time and runtime check to detect this change.

@vtjnash Ref #13787
@tkelman This should make the code more MSVC compatible but it's also possible that I break something without realizing it....

tkelman · 2015-11-11T02:24:49Z

built fine on msvc straight away, nicely done.

vtjnash · 2015-11-11T04:01:57Z

src/codegen.cpp

    struct {
        int64_t isref;
        Function *f;
-    } data[];
+    } data[1];


I'm pretty sure this change isn't ISO-conforming C. The old version should have been more correct.

What is not ISO-conforming about this?

The warning I get is

yuyichao% LANG=C g++ -pedantic -x c++ -Wall -Wextra - -c -o /dev/null <<EOF typedef struct { int a; int b[]; } A; EOF <stdin>:3:8: warning: ISO C++ forbids zero-size array 'b' [-Wpedantic]

OK so I find this stackoverflow thread. And it seems that there basically isn't a ISO C++ conforming way to do this.

The flexible member is invalid C++ syntax and causes warning under -pedantic.

The single element array could technically be UB but It's not clear if it's legal or portable, but it is rather popular. and does seem to work under all known implementations.... (and also issue no warnings.)

does seem to work under all known implementations

http://stackoverflow.com/a/10500898

perhaps we should just switch this struct to a std::unordered_map< uint64_t, Function* >

Doing the [1] thing can now give you runtime errors on OS X, cf #13845

perhaps we should just switch this struct to a std::unordered_map< uint64_t, Function* >

We don't require c++11 in general yet but it could be std::map? Looks like it currently is simply a std::vector?

std::vector< std::pair< int64_t, Function* > > would also work. it's expected to be a very small set, so actually it's best not to use unordered_map, which may significantly over-allocate.

yuyichao · 2015-11-11T19:07:22Z

I updated the PR to not use [1] and calculate the addresses directly instead. I've implemented most of what @vtjnash suggested except,

Use std::vector or std::map for cFunctionList. It's requires slightly more changes to implement and introduce one more indirection since we can only store a pointer in the C structure.
Define jl_value_t as a union. It's largely independent of this PR.

The static inline version of jl_symbol_name_ is to provide better type check. jl_sym_t::name is used a lot in the code and usually in the form of ...->name->name (sometimes ...->name->name->name) and is really likely to get wrong if there's not a strong type check.

vtjnash · 2015-11-11T20:46:35Z

lgtm

tkelman · 2015-11-11T21:09:09Z

6db03d0 looks like it also works on msvc. Do any of the embedding docs or examples need updating for this?

yuyichao · 2015-11-11T21:18:30Z

Do any of the embedding docs or examples need updating for this?

Good question. All the changes are API level details (i.e. shouldn't break ABI) and it seems that none of these are covered by the embedding doc. (We don't have doc for the various jl_*_t types).

yuyichao · 2015-11-11T21:23:29Z

Rebased to resolve conflict with #13787 .... I guess I shouldn't have silently included the fix for the variable not used warning when assert is off....

vtjnash · 2015-11-11T21:59:40Z

Oops, i guess at least one of us shouldn't have silently added that fix. We very nearly came up with the same patch too :/

yuyichao · 2015-11-13T21:41:24Z

Rebased and add a short comment about jl_symbol_name_. Will merge tomorrow if there's no other issues.

Remove 0-size and variable size array members

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

vtjnash reviewed Nov 11, 2015
View reviewed changes

yuyichao force-pushed the yyc/gcc-warn branch from f8431c0 to 6db03d0 Compare November 11, 2015 18:31

yuyichao force-pushed the yyc/gcc-warn branch from 6db03d0 to df7febe Compare November 11, 2015 21:22

Remove 0-size and variable size array members from public headers.

63f3edf

yuyichao force-pushed the yyc/gcc-warn branch from df7febe to 63f3edf Compare November 13, 2015 21:35

yuyichao added a commit that referenced this pull request Nov 14, 2015

Merge pull request #13945 from JuliaLang/yyc/gcc-warn

9c9a180

Remove 0-size and variable size array members

yuyichao merged commit 9c9a180 into master Nov 14, 2015

yuyichao deleted the yyc/gcc-warn branch November 14, 2015 12:53

yuyichao mentioned this pull request Nov 15, 2015

Fix compile errors and warnings in gc debugging code #14003

Merged

vtjnash mentioned this pull request Nov 15, 2015

Prevent flisp from triggering the Darwin C library's overflow check #13845

Merged

yuyichao added a commit that referenced this pull request Jan 28, 2016

Fix -Wpedantic and enable it in c++ code.

0995d3a

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

yuyichao added a commit that referenced this pull request Jan 28, 2016

Fix -Wpedantic and enable it in c++ code.

6fa249b

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

yuyichao mentioned this pull request Jan 28, 2016

Fix -Wpedantic and enable it in c++ code #14822

Merged

yuyichao added a commit that referenced this pull request Jan 28, 2016

Fix -Wpedantic and enable it in c++ code.

cc73a2d

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

nalimilan mentioned this pull request Jan 28, 2016

[release-0.4, RFC]: Remove 0-size and variable size array members from public headers. #14829

Merged

yuyichao added a commit that referenced this pull request Jan 28, 2016

Fix -Wpedantic and enable it in c++ code.

62e9cdf

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

yuyichao added a commit that referenced this pull request Jan 28, 2016

Fix -Wpedantic and enable it in c++ code.

e44d681

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

yuyichao added a commit that referenced this pull request Jan 28, 2016

Fix -Wpedantic and enable it in c++ code.

b478af2

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

yuyichao added a commit that referenced this pull request Jan 29, 2016

Fix -Wpedantic and enable it in c++ code.

02b0430

This flag is added by llvm-config 3.7 on ArchLinux. Ref #13945

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove 0-size and variable size array members #13945

Remove 0-size and variable size array members #13945

yuyichao commented Nov 11, 2015

tkelman commented Nov 11, 2015

vtjnash Nov 11, 2015

yuyichao Nov 11, 2015

yuyichao Nov 11, 2015

vtjnash Nov 11, 2015

vtjnash Nov 11, 2015

Keno Nov 11, 2015

yuyichao Nov 11, 2015

vtjnash Nov 11, 2015

yuyichao commented Nov 11, 2015

vtjnash commented Nov 11, 2015

tkelman commented Nov 11, 2015

yuyichao commented Nov 11, 2015

yuyichao commented Nov 11, 2015

vtjnash commented Nov 11, 2015

yuyichao commented Nov 13, 2015

Remove 0-size and variable size array members #13945

Remove 0-size and variable size array members #13945

Conversation

yuyichao commented Nov 11, 2015

tkelman commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyichao commented Nov 11, 2015

vtjnash commented Nov 11, 2015

tkelman commented Nov 11, 2015

yuyichao commented Nov 11, 2015

yuyichao commented Nov 11, 2015

vtjnash commented Nov 11, 2015

yuyichao commented Nov 13, 2015